NW6 | Rabia Avci | JS1 Module | [TECH ED] Complete week 4 exercises | Week 4#177
NW6 | Rabia Avci | JS1 Module | [TECH ED] Complete week 4 exercises | Week 4#177RbAvci wants to merge 19 commits intoCodeYourFuture:mainfrom
Conversation
percentage-change time-format
done is-valid-triangle
done get-card-value
Ara225
left a comment
Ara225
left a comment
There was a problem hiding this comment.
Good work, I'm moaning about your loops quite a bit. I think you should have a look at loops and some of the array functions again and make sure that you understand
| } | ||
| if (count === 16) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Good work, like the comments as well
Just a small comment (not a telling off or something to be fixed) Loops can be quite bad performance wise and this code isn't the easiest to read, so in the real world I'd be looking hard for shortcuts when I came to refactor. One that springs to mind is
let badCardNum = cardNum[0] * 16;
if (cardNum === badCardNum) {
return false;
| } | ||
| if (total < 16) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Works perfectly and appreciate the comments here, afraid I have two of my own.
- Don't use the same varible for index - it causes bugs something like this is better
while (let index ....)This way each loop manages it's own index. - There is a simpler way of doing this - genrally speaking when there's something like this that is often done, there will be (you'll develop a nose for this eventually)
I would use .split() to make the string into an array https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/split and then call .reduce() on the array https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/reduce
| const currentOutput2 = isValidCardNumber(num2); | ||
|
|
||
| //THEN | ||
| expect(currentOutput1).toBe(targetOutput); |
There was a problem hiding this comment.
Nice tests - overall good work needs a little refactoring I think
| let count = 0; | ||
| let index = 0; | ||
|
|
||
| while (index < str.length){ |
There was a problem hiding this comment.
Good work. I do want to see you stopping defining index outside of the loop though. I would also have a look at diffrent types of loops - a for ... in loop would be a better and easier one to use here. https://buzzcoder.gitbooks.io/codecraft-javascript/content/string/loop-through-a-string.html
|
|
||
| test("works for any other number", function () { | ||
| expect(getOrdinalNumber(7)).toBe("7th"); | ||
| expect(getOrdinalNumber(99)).toBe("99th"); |
| // c) Why is index++ being used? | ||
| // to check/traverse all the characters in str | ||
| // d) What is the condition index < str.length used for? | ||
| // to define when to stop checking characters |
| return `${"Straight angle"}`; | ||
| } | ||
| else if (angle>180 && angle<360){ | ||
| return `${"Reflex angle"}`; |
There was a problem hiding this comment.
All correct. I would avoid using backticks here; they aren't needed, you can just return a string
| return 10; | ||
| } | ||
| throw Error("Invalid card rank."); | ||
| } |
There was a problem hiding this comment.
Simple effective solution with excellent testing 👍
| if (numerator < denominator) { | ||
| return true; | ||
| } else if (numerator > denominator || numerator === denominator) { | ||
| return false; |
| if (a <= 0 || b <= 0 || c <= 0) { | ||
| return false; | ||
| } | ||
| if (a + b <= c || a + c <= b || b + c <= a) { |
2 participants
Learners, PR Template
Self checklist
Changelist
Rewrite section (as written in Read me) is done, and get used to writing tests.
Implement section completed (except password-validator).
Questions